Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

server: fix ComStmtSendLongData when data length is 0 #7485

Merged
merged 3 commits into from
Aug 27, 2018
Merged

server: fix ComStmtSendLongData when data length is 0 #7485

merged 3 commits into from
Aug 27, 2018

Conversation

jackysp
Copy link
Member

@jackysp jackysp commented Aug 24, 2018 •
edited
Loading

What problem does this PR solve?

setCharacterStream(1, new StringReader(""), 0); this API in JDBC will use ComStmtSendLongData to send a long string. When the data length is 0, TiDB cannot distinguish whether there is a parameter with no data or there is no parameter at all.

What is changed and how it works?

This pr appends an extra 0 at the end of the bound parameter. In ComStmtExecute, we use it to distinguish if there is a parameter or not.
(How does MySQL deal with this?
MySQL set the length of the parameter as len(data - 6), which 6 is the length of the header of the request.)

Check List

Tests

  • Integration test (JDBC test cases)

Code changes

  • Has exported function/method change

Side effects

  • Increased code complexity

Related changes

  • Need to cherry-pick to the release branch

PTAL @coocood

zimulala
zimulala reviewed Aug 24, 2018
View reviewed changes
Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@zimulala zimulala added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 24, 2018
winkyao
winkyao previously approved these changes Aug 25, 2018
View reviewed changes
Copy link
Contributor

@winkyao winkyao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jackysp
Copy link
Member Author

jackysp commented Aug 27, 2018

/run-all-tests

@coocood
Copy link
Member

coocood commented Aug 27, 2018

@jackysp

This is a way to solve the problem, but may not the best.
The problem is that a nil slice appending a zero-length slice is still nil slice.

We can make it work like this in TiDBStatement

	if len(data) == 0 {
		ts.boundParams[paramID] = []byte{}
	} else {
		ts.boundParams[paramID] = append(ts.boundParams[paramID], data...)
	}

@jackysp
Copy link
Member Author

jackysp commented Aug 27, 2018

/run-all-tests

@jackysp jackysp added status/all tests passed and removed status/LGT1 Indicates that a PR has LGTM 1. labels Aug 27, 2018
@jackysp
Copy link
Member Author

jackysp commented Aug 27, 2018

PTAL @coocood

@jackysp jackysp added the status/LGT1 Indicates that a PR has LGTM 1. label Aug 27, 2018
@coocood
Copy link
Member

coocood commented Aug 27, 2018

LGTM

@coocood coocood merged commit 09fb68a into pingcap:master Aug 27, 2018
jackysp added a commit to jackysp/tidb that referenced this pull request Aug 28, 2018
@jackysp
server: fix ComStmtSendLongData when data length is 0 (pingcap#7485)
760740f 
(cherry picked from commit 09fb68a)
@jackysp jackysp mentioned this pull request Aug 28, 2018
Merged
ngaut pushed a commit that referenced this pull request Aug 28, 2018
@jackysp @ngaut
server: fix ComStmtSendLongData when data length is 0 (#7485) (#7508)
d677534
@jackysp jackysp deleted the fix_binary_long_data branch September 7, 2018 11:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zimulala zimulala zimulala left review comments

@coocood coocood coocood approved these changes

@winkyao winkyao winkyao left review comments

Assignees
No one assigned
Labels
component/mysql-protocol status/LGT1 Indicates that a PR has LGTM 1.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jackysp @coocood @zimulala @winkyao